Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

⚠ Migrate to ginkgo v2 #1977

Merged
merged 15 commits into from
Aug 26, 2022
Merged

Conversation

schrej
Copy link
Member

@schrej schrej commented Aug 11, 2022

Revives #1792, thanks to @acumino for all your work on this.

Fixes: #1545

Important Note:
-> Per node timeout has been removed to prevent the wrongly marking a test as failure which is caused by some other test. Read more about it here
-> Custom reporters have been deprecated in ginkgo v2 and will be removed in future releases.

Note: This is a breaking change for two reasons:

  • (we're not affected by this) ginkgo v1 and v2 have conflicting flags, so you can't import them both at the same time (see here). Thanks to @mboersma for pointing this out
  • The PR removes two helpers in pkg/envtest/printer that were used with ginkgo v1. Since you can't use both versions at the same time, this isn't relevant though.

Upstream upgraded already: kubernetes/kubernetes#109111

/cc @matteoolivi @camilamacedo86

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2022
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 11, 2022
@k8s-ci-robot
Copy link
Contributor

@schrej: GitHub didn't allow me to request PR reviews from the following users: matteoolivi.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @matteoolivi @camilamacedo86

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@alvaroaleman alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 11, 2022
@alvaroaleman
Copy link
Member

ginkgo v1 and v2 have conflicting flags, so you can't import them both at the same time (see onsi/ginkgo#875 (comment)). Thanks to @mboersma for pointing this out

But we only import it in _test.go files and in the printer package, so this shouldn't matter for consumers, right?

@schrej
Copy link
Member Author

schrej commented Aug 12, 2022

But we only import it in _test.go files and in the printer package, so this shouldn't matter for consumers, right?

I just tested with the cluster-api project, manually replacing controller-runtime with this branch there. Tests can still run, even though CAPI is still on ginkgo v1. So you're right, it shouldn't matter to consumers in our case.

I'll revert the removal of the two helpers in pkg/envtest/printer, which should make this a non-breaking change then.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 12, 2022
@schrej schrej force-pushed the ginkgo-v2 branch 2 times, most recently from 2cdde96 to ccc9e3d Compare August 12, 2022 15:18
@schrej schrej force-pushed the ginkgo-v2 branch 3 times, most recently from 4b246c2 to 3b34d95 Compare August 17, 2022 10:20
@schrej
Copy link
Member Author

schrej commented Aug 17, 2022

Ok. This is now using the ginkgo cli, which allows to remove the ginkgo helper to create unit test reports.
The apidiff is failing because of that.

I've adapted the test scripts to install the ginkgo cli automatically and to copy the unit test report to the correct location for prow to detect and visualise it.

One major drawback of the ginkgo cli is that it does not cache tests, and always runs the full suite.

@schrej
Copy link
Member Author

schrej commented Aug 17, 2022

@schrej the tests paniced but still passed: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_controller-runtime/1977/pull-controller-runtime-test-master/1559847725640454144

Yeah. Apparently ginkgo detects and reports panics even when they are intentional ("injected panic").

•••E0817 10:24:49.659771   23457 runtime.go:77] Observed a panic: injected panic

The k8s.io/klog package logs panics it observes to stderr, and Ginkgo apparently prints that even when the test passes.

@pohly
Copy link

pohly commented Aug 22, 2022

The k8s.io/klog package logs panics it observes to stderr, and Ginkgo apparently prints that even when the test passes.

It's the runtime which calls klog.Error, which then seems to go to stderr.

Does the suite call klog.SetOutput(ginkgo.GinkgoWriter)? Then I think you can suppress that extra output to stderr by reconfiguring the "stderrthreshold". I have a pending commit which does that for the in-tree e2e.test:
kubernetes/kubernetes@7d6af86

@pohly
Copy link

pohly commented Aug 22, 2022

I've adapted the test scripts to install the ginkgo cli automatically and to copy the unit test report to the correct location for prow

FYI, it is possible to enable JUnit output without using the ginkgo cli. You can either do go test ./<test dir> -args -ginkgo.junit-report=<output file> or add some code like https://github.com/kubernetes/kubernetes/blob/a1128e380c2cf1c2d7443694673d9f1dd63eb518/test/e2e/framework/test_context.go#L558-L607.

@alvaroaleman
Copy link
Member

FYI, it is possible to enable JUnit output without using the ginkgo cli. You can either do go test ./ -args -ginkgo.junit= or add some code like

The issue with that was that it made the tests always exit with rc0 even when there are failures

@pohly
Copy link

pohly commented Aug 22, 2022

The issue with that was that it made the tests always exit with rc0 even when there are failures

Which of the two options? -ginkgo.junit or adding some custom GenerateJUnitReport invocation? Either way, that sounds like a bug that should be reported.

@pohly
Copy link

pohly commented Aug 22, 2022

Hmm, both works for me (PMEM-CSI test/e2e, using ginkgo v2 v2.1.4).

FYI, I have to use this fork of the controller-runtime for that project because otherwise I would be stuck at ginkgo v1 and couldn't update to Kubernetes 1.25. I'll have to keep that in a WIP PR until this PR gets merged and released.

@alvaroaleman
Copy link
Member

Which of the two options? -ginkgo.junit or adding some custom GenerateJUnitReport invocation? Either way, that sounds like a bug that should be reported.

It was reported here, maybe it doesn't happen anymore? onsi/ginkgo#706

FYI, I have to use this fork of the controller-runtime for that project because otherwise I would be stuck at ginkgo v1 and couldn't update to Kubernetes 1.25. I'll have to keep that in a WIP PR until this PR gets merged and released.

Why? Shouldn't we only depend on ginkgo in _test.go files? If anyhow possible, we shouldn't force users to use either version

@schrej
Copy link
Member Author

schrej commented Aug 22, 2022

Why? Shouldn't we only depend on ginkgo in _test.go files? If anyhow possible, we shouldn't force users to use either version

Probably because we're currently importing ginkgo v1 in pkg/envtest/printer.

@alvaroaleman
Copy link
Member

Probably because we're currently importing ginkgo v1 in pkg/envtest/printer.

But v2 is imported as github.com/onsi/ginkgo/v2 or not?

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2022
@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit 2c3a6fa into kubernetes-sigs:master Aug 26, 2022
@k8s-ci-robot k8s-ci-robot added this to the v0.10.x milestone Aug 26, 2022
@schrej schrej deleted the ginkgo-v2 branch August 26, 2022 09:03
ArangoGutierrez added a commit to ArangoGutierrez/node-feature-discovery-operator that referenced this pull request Jan 16, 2023
As seen in:
kubernetes-sigs/controller-runtime#1977

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
nixpanic added a commit to csi-addons/kubernetes-csi-addons that referenced this pull request Jan 31, 2023
…ckage

controller-runtime does not provide the pkg/envtest/printer package
anymore since it was migrated to Ginkgo v2. Using the package prevents
kubernetes-csi-addons from moving to a more recent version of
controller-runtime (and other dependencies).

See-also: kubernetes-sigs/controller-runtime#1977
Signed-off-by: Niels de Vos <ndevos@redhat.com>
mergify bot pushed a commit to csi-addons/kubernetes-csi-addons that referenced this pull request Jan 31, 2023
…ckage

controller-runtime does not provide the pkg/envtest/printer package
anymore since it was migrated to Ginkgo v2. Using the package prevents
kubernetes-csi-addons from moving to a more recent version of
controller-runtime (and other dependencies).

See-also: kubernetes-sigs/controller-runtime#1977
Signed-off-by: Niels de Vos <ndevos@redhat.com>
yevgeny-shnaidman pushed a commit to yevgeny-shnaidman/cluster-nfd-operator that referenced this pull request Feb 1, 2023
As seen in:
kubernetes-sigs/controller-runtime#1977

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
ArangoGutierrez added a commit to ArangoGutierrez/node-feature-discovery-operator that referenced this pull request Mar 19, 2023
As seen in:
kubernetes-sigs/controller-runtime#1977

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
cburchett added a commit to NetApp-Learning-Services/gateway that referenced this pull request May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ginkgo v2 migration
7 participants